Repository navigation
fix: replace busy-wait loop in BallThread with wait/notify (#2977) - #3614
KANISHKMAKKAR wants to merge 7 commits into
Conversation
PR SummaryReplaced the busy-wait loop in Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (2)
Files Processed (2)
- twin/README.md (1 hunk)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (2 hunks)
Actionable Comments (0)
Skipped Comments (6)
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [39-39]
best_practice: "Introduce a dedicated synchronization lock for suspend/resume"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [50-55]
best_practice: "Guard suspend state with wait loop"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [68-70]
best_practice: "Synchronize suspendMe state update"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [75-78]
best_practice: "Wake up thread on resume"
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [83-87]
enhancement: "Ensure stop notifies waiting thread"
-
twin/README.md [91-93]
best_practice: "README aligns with implementation"
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3614 +/- ##
============================================
+ Coverage 84.06% 84.08% +0.01%
- Complexity 4353 4355 +2
============================================
Files 1133 1133
Lines 15400 15413 +13
Branches 739 740 +1
============================================
+ Hits 12946 12960 +14
+ Misses 2158 2157 -1
Partials 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 61b12ed: Update BallThreadTest.java
Files Processed (2)
- twin/README.md (1 hunk)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
twin/README.md [91-137]
enhancement: "Introduce wait/notify synchronization for suspension in BallThread."
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [117-129]
enhancement: "Test: stop while suspended does not interact with BallItem."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 8356b1e: Update BallThreadTest.java
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [135-137]
best_practice: "Flaky wait-for-WAITING loop in test"
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [161-161]
best_practice: "Deterministic synchronization before interrupt"
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [140-141]
maintainability: "Assert termination state after join"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a6cc3ef: Update BallThreadTest.java
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [114-144]
maintainability: "Avoid busy-waiting when waiting for thread state."
-
twin/src/test/java/com/iluwatar/twin/BallThreadTest.java [145-166]
maintainability: "Interruption handling test robustness."
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 568f395: Update BallThreadTest.java
Files Processed (1)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 07e2e3c: Update BallThreadTest.java
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
This branch has not been deployed
1 participant
Addresses item 2 (
Twin / BallThread.java) of #2977.Problem
While suspended,
BallThreadwakes every 250 ms only to re-checkisSuspendedand do nothing. BecauseresumeMe()merely flips the flag, it has no way to wake the sleeping thread, so resuming is delayed until the current sleep tick expires.stopMe()has the same weakness.Change
isSuspendedis now guarded by a private lock. The run loop waits on that lock while suspended, andresumeMe()/stopMe()callnotifyAll(). Polling is replaced by blocking, and resume takes effect immediately.The
Thread.sleep(250)animation pacing is intentional and left unchanged.Result
Latency from
resumeMe()to the firstdraw(), averaged over 5 runs:Notes
BallThreadTest(suspend / resume / interrupt) passes unmodified, including theverifyNoMoreInteractionsassertions and theRuntimeException-on-interrupt check.notifyAll()instopMe()is required: without it, stopping an already-suspended thread would block forever inwait().twin/README.mdembeds this class verbatim, so it is updated to match.Retryclasses use deliberate backoff sleeps rather than busy-waiting.